fix: preserve aggregate filter pushdown order - #22926
Conversation
github-actions
Bot
added
core
|
Hi, this PR fixes #22925 and is still waiting for review. The only failing check is Thanks! |
05e5a77 to
3626a96
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #22926 +/- ##
==========================================
- Coverage 80.70% 80.70% -0.01%
==========================================
Files 1095 1095
Lines 372554 372537 -17
Branches 372554 372537 -17
==========================================
- Hits 300665 300640 -25
- Misses 53921 53925 +4
- Partials 17968 17972 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
neilconway
left a comment
neilconway
left a comment
There was a problem hiding this comment.
Thank you for this PR! Overall, this is excellent work: clear diagnosis, minimal and correct fix, and good test coverage. I'm sorry it didn't get reviewed sooner -- a few minor suggestions but it should be good to land shortly.
3626a96 to
4b0e6c6
Compare
neilconway
left a comment
neilconway
left a comment
There was a problem hiding this comment.
Looking good! Can you update the PR description, please? Currently refers to the old implementation; would also be good to elaborate on the pushdown change for global aggregates?
| )) | ||
| } | ||
|
|
||
| fn assert_parent_filter_remains_above_aggregate(plan: Arc<dyn ExecutionPlan>) { |
There was a problem hiding this comment.
This passes without the functional changes in this PR; I think you need to enable pushdown_filters instead of using the default config.
3 participants
Which issue does this PR close?
Rationale for this change
FilterPushdownmaps child pushdown results back to the original parent filters by position.AggregateExec::gather_filters_for_pushdownwas splitting incoming parent filters into safe and unsafe buckets, building the child filter description from the safe filters, and then appending unsupported unsafe filters. For mixed predicates this reordered the parent filter results.For example, for a predicate above an aggregate such as
cnt@2 = 1 AND b@1 = bar, the filter on the grouping columnbcan be pushed below the aggregate, but the filter on aggregate outputcntmust remain above it. If the returned parent filter results are reordered, the optimizer can treat the pushed-down grouping-column filter result as if it belonged to the aggregate-output filter and incorrectly remove the aggregate-output filter.What changes are included in this PR?
parent_filtersorder inAggregateExec::gather_filters_for_pushdown.AggregateExec::gather_filters_for_pushdownpath is exercised end-to-end.Are these changes tested?
Yes:
cargo fmt --allcargo fmt --all -- --checkcargo test -p datafusion --test core_integration physical_optimizer::filter_pushdown::test_pushdown_through_aggregatescargo test --test sqllogictests -- push_down_filter_regression:528cargo test --test sqllogictests -- push_down_filter_regressioncargo clippy --all-targets --all-features -- -D warningsAre there any user-facing changes?
No.